fix grpc endpoint parse when no dns scheme is provided#822
fix grpc endpoint parse when no dns scheme is provided#822
Conversation
Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #822 +/- ##
=======================================
Coverage 54.69% 54.69%
=======================================
Files 51 51
Lines 3474 3474
=======================================
Hits 1900 1900
Misses 1434 1434
Partials 140 140 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes gRPC endpoint parsing so addresses that don’t explicitly specify a dns scheme are dialed as bare host:port targets (passthrough resolver), avoiding failures caused by gRPC target parsing/resolution behavior in the current dependency set.
Changes:
- Track whether the original endpoint string omitted a scheme and, in that case, return
host:portinstead ofdns:host:port. - Preserve existing
dns:/dns://authority/...target formats when the caller explicitly selects thednsscheme. - Update parsing unit tests to reflect the new expected dial targets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| client/internal/parse.go | Adjusts ParseGRPCEndpoint to return passthrough-style targets when no scheme is provided, while keeping explicit dns behavior. |
| client/internal/parse_test.go | Updates expected targets in test cases to match the new parsing output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/internal/parse.go
Outdated
| // Use passthrough resolver (just host:port) instead of dns: scheme. | ||
| // gRPC v1.78's DNS resolver is broken with grpc.DialContext() — it | ||
| // never fires regardless of URI format. The passthrough resolver | ||
| // works correctly and grpc.DialContext() defaults to it for bare | ||
| // host:port targets. |
There was a problem hiding this comment.
The comment says gRPC v1.78 “DNS resolver is broken with grpc.DialContext() — it never fires regardless of URI format”, but this function still returns dns:... / dns://... targets when the caller explicitly uses the dns scheme. That makes the wording look broader than the actual behavior being worked around and could confuse future maintenance.
Consider rephrasing to the specific observed failure mode (e.g., dns:host:port being parsed as an opaque target / resolver not invoked for that form) and optionally link to an upstream gRPC issue or repo issue for context.
| // Use passthrough resolver (just host:port) instead of dns: scheme. | |
| // gRPC v1.78's DNS resolver is broken with grpc.DialContext() — it | |
| // never fires regardless of URI format. The passthrough resolver | |
| // works correctly and grpc.DialContext() defaults to it for bare | |
| // host:port targets. | |
| // Use passthrough resolver (just host:port) instead of the dns: scheme. | |
| // In gRPC v1.78, targets of the form dns:host:port can be treated as | |
| // opaque by grpc.DialContext(), so the DNS resolver is not invoked. | |
| // Using a bare host:port target makes grpc.DialContext() fall back to | |
| // the passthrough resolver, which resolves and connects correctly. |
Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com>
Description
I ran into this issue the other day with the dapr API exposed in
*.7f000001.nip.io:30443(which is equivalent to *.127.0.0.1.nip.io btw)Without this fix I got this
With the fix I got this
FYI running my apps with the env vars
GRPC_GO_LOG_VERBOSITY_LEVEL=99 GRPC_GO_LOG_SEVERITY_LEVEL=infoto get these log outputsIssue reference
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: